-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable uninstalling requests
#194
Conversation
|
the problem is that we have no way of making dependencies As a result we decided to have internet packages in the dependencies. The reason for that is simple: no one would opt-in for telemetry, thats is normal for any product and software - even if the only target group are our company-internal internal users I think no one would opt in. Telemetry packages were never optional to install, there was an optional dependency group which was however referenced in the main deps (#181 merely removed that because poetry cant deal with that) For externals, we had hoped that
would suffice. But I can see that if you want to be really sure you'd also want to not have any internet package in your environment. If that is what your PR enables then I'd be willing to accept it. I guess it would still require you to do some manual uninstalling after baybe installation or how are you dealing with it? I think the telemetry mention was moved from the README because we wanted to reduce content there. But if it helps we can reintroduce the mention or at least a link to the detailed explanation. |
Thanks for the quick and detailed answer. I see the points about this being open source (which is awesome!) but I think unfortunately not everyone will take the time to read it. Why not make it opt-in? |
@rickwierenga well, just briefly thinking about the amount of newsletters / telemetry measurements I have Until we find a better way to enable |
Sounds good! I'll try this out for my experiments this night and tomorrow, but if results are good I'm happy to work on this! |
Hi @rickwierenga 👋 thanks for connecting. I think @Scienfitz explained our dilemma quite accurately. To provide a little more context: baybe was originally developed purely for inhouse optimization, but we pushed to make the library open-source and were in fact successful with this plan. However, has now brought us into a sort of chimera state where the telemetry part is still "inhouse" in a sense. And I think you will understand that we can't simply drop it because the gathered internal usage stats eventually justify our work on the library – without them, there would have been no package in the first place. That said, we are hoping to find a better solution in the mid term, ideally via opt-out. In fact, there is an ongoing discussion on extending pip capabilities in that direction, but it hasn't yet progressed very far. Let's see what the happens in the next few months. Until then, I think your PR is a good first step 👍🏼 |
@rickwierenga were you able to test whether this works for you, ie uninstalling the internet deps + your code change? If you confirm I will add updates to the telemetry descriptions and merge this |
Hi @rickwierenga, the other PR #205 that adds a telemetry section to the README is up. Ideally, we'd like to wait for your final confirmation before merging both. Can you give a quick update if everything works as expected for you? |
Hi @Scienfitz, seems like @rickwierenga is currently busy with other things. Could you include that additional paragraph to the readme so that we can proceed towards merging? |
@AdrianSosic will do so but I will check whether the uninstall actually enables what was claimed before I merge |
Jep, makes sense. Let me know if I should do something... |
nice! I like the new transparency :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested, proceeding to merge now
Hi @rickwierenga But if you want this PR merged with your association you still need to sign the CLA agreement, as pointed out by the bot (second comment in this PR) |
Hi @rickwierenga, just briefly wanted to inform you that we plan to merge this PR before releasing 0.9.0, which will happen at some point during this week. We'd love to have your association with this PR, so I encourage you to sign the CLA the latest until tomorrow. Of course, you don't need to sign if you refuse, but then we'll probably have to merge these changes from a different branch. So hoping you will accept 🙃 |
@rickwierenga We recently removed the |
I was surprised to learn BayBE includes telemetry so deep in the project. It's uncomfortable importing a package that talks to the internet.
Since
these dependencies seem useless for most users?
In this PR, I moved
requests
to the# Attempt telemetry import
section so it's no longer needed to run anything in this project.Because of #181, the telemetry is no longer optional to install when using pyproject.toml? I just installed the dependencies manually. It'd be nice to have this be optional so users don't get surprised by an automatic requests installed. Does poetry not do that?
Also, is there a reason this is not mentioned in the README? It used to be, but was removed: #3. Why?